libostree: Rework OstreeAsyncProgress to use GVariants internally

OstreeAsyncProgress currently does some contortions to try and avoid
allocating space for guints and guint64s (on 64-bit platforms), but this
means it uses two GHashTables. A GHashTable allocates 8 buckets even
when empty. Given that the largest usage of OstreeAsyncProgress in
libostree puts 13 uints and 5 uint64s in it, this optimisation does not
save significant (if any) memory.

Instead, change OstreeAsyncProgress to store values internally as
GVariants, and expose this with some new API:
 • ostree_async_progress_get_variant()
 • ostree_async_progress_set_variant()
Each GVariant is allocated on the heap. As they are immutable, they are
thread-safe once returned by a getter.

The existing API continues to work as before, except in the case where a
key is set/got as both a uint and a uint64 — there will now be a
collision (and a GVariant type checking failure) whereas previously
there was no collision. Nothing in OSTree uses OstreeAsyncProgress this
way though.

The new API can be used to share more complex data via the progress API.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
Closes: #819
Approved by: cgwalters

diff --git a/apidoc/ostree-sections.txt b/apidoc/ostree-sections.txt
index e6ed4c8a9c4dff619617fd929ab2e8c550e76ebc..5625a1936d740c1712498e63e148f59badf1c0b9 100644 (file)
--- a/apidoc/ostree-sections.txt
+++ b/apidoc/ostree-sections.txt
@@ -4,9 +4,11 @@ OstreeAsyncProgress
 ostree_async_progress_new
 ostree_async_progress_new_and_connect
 ostree_async_progress_get_status
+ostree_async_progress_get_variant
 ostree_async_progress_get_uint
 ostree_async_progress_get_uint64
 ostree_async_progress_set_status
+ostree_async_progress_set_variant
 ostree_async_progress_set_uint
 ostree_async_progress_set_uint64
 ostree_async_progress_finish

diff --git a/src/libostree/libostree.sym b/src/libostree/libostree.sym
index d9339bf23962cc22efa23f9305377d28894301ee..0787d87722efdf6fa479aa31899cd9b35f1bf716 100644 (file)
--- a/src/libostree/libostree.sym
+++ b/src/libostree/libostree.sym
@@ -394,13 +394,11 @@ global:
  *                         NOTE NOTE NOTE
  */
 
-/* Uncomment when adding the first new symbol */
-/*
-LIBOSTREE_2017.$NEWVERSION {
+LIBOSTREE_2017.6 {
 global:
-       someostree_symbol_deleteme;
+  ostree_async_progress_get_variant;
+  ostree_async_progress_set_variant;
 } LIBOSTREE_2017.4;
-*/
 
 /* Stub section for the stable release *after* this development one; don't
  * edit this other than to update the last number.  This is just a copy/paste

diff --git a/src/libostree/ostree-async-progress.c b/src/libostree/ostree-async-progress.c
index 0851fd8673521347abdb05621fc3af9511f9ffd9..68f4c8349f8a89db88ea121b8234f153fadeeaf3 100644 (file)
--- a/src/libostree/ostree-async-progress.c
+++ b/src/libostree/ostree-async-progress.c
@@ -22,6 +22,8 @@
 
 #include "ostree-async-progress.h"
 
+#include "libglnx.h"
+
 /**
  * SECTION:ostree-async-progress
  * @title: Progress notification system for asynchronous operations
@@ -39,12 +41,6 @@
  * operation.
  */
 
-#if GLIB_SIZEOF_VOID_P == 8
-#define _OSTREE_HAVE_LP64 1
-#else
-#define _OSTREE_HAVE_LP64 0
-#endif
-
 enum {
   CHANGED,
   LAST_SIGNAL
@@ -59,8 +55,7 @@ struct OstreeAsyncProgress
   GMutex lock;
   GMainContext *maincontext;
   GSource *idle_source;
-  GHashTable *uint_values;
-  GHashTable *uint64_values;
+  GHashTable *values;  /* (element-type uint GVariant) */
 
   gboolean dead;
 
@@ -79,8 +74,7 @@ ostree_async_progress_finalize (GObject *object)
   g_mutex_clear (&self->lock);
   g_clear_pointer (&self->maincontext, g_main_context_unref);
   g_clear_pointer (&self->idle_source, g_source_unref);
-  g_hash_table_unref (self->uint_values);
-  g_hash_table_unref (self->uint64_values);
+  g_hash_table_unref (self->values);
   g_free (self->status);
 
   G_OBJECT_CLASS (ostree_async_progress_parent_class)->finalize (object);
@@ -114,46 +108,53 @@ ostree_async_progress_init (OstreeAsyncProgress *self)
 {
   g_mutex_init (&self->lock);
   self->maincontext = g_main_context_ref_thread_default ();
-  self->uint_values = g_hash_table_new (NULL, NULL);
-#if _OSTREE_HAVE_LP64
-  self->uint64_values = g_hash_table_new (NULL, NULL);
-#else
-  self->uint64_values = g_hash_table_new_full (NULL, NULL,
-                                               NULL, g_free);
-#endif
+  self->values = g_hash_table_new_full (NULL, NULL, NULL, (GDestroyNotify) g_variant_unref);
 }
 
-guint
-ostree_async_progress_get_uint (OstreeAsyncProgress       *self,
-                                const char                *key)
+/**
+ * ostree_async_progress_get_variant:
+ * @self: an #OstreeAsyncProgress
+ * @key: a key to look up
+ *
+ * Look up a key in the #OstreeAsyncProgress and return the #GVariant associated
+ * with it. The lookup is thread-safe.
+ *
+ * Returns: (transfer full) (nullable): value for the given @key, or %NULL if
+ *    it was not set
+ * Since: 2017.6
+ */
+GVariant *
+ostree_async_progress_get_variant (OstreeAsyncProgress *self,
+                                   const char          *key)
 {
-  guint rval;
+  GVariant *rval;
+
+  g_return_val_if_fail (OSTREE_IS_ASYNC_PROGRESS (self), NULL);
+  g_return_val_if_fail (key != NULL, NULL);
+
   g_mutex_lock (&self->lock);
-  rval = GPOINTER_TO_UINT (g_hash_table_lookup (self->uint_values,
-                                                GUINT_TO_POINTER (g_quark_from_string (key))));
+  rval = g_hash_table_lookup (self->values, GUINT_TO_POINTER (g_quark_from_string (key)));
+  if (rval != NULL)
+    g_variant_ref (rval);
   g_mutex_unlock (&self->lock);
+
   return rval;
 }
 
+guint
+ostree_async_progress_get_uint (OstreeAsyncProgress       *self,
+                                const char                *key)
+{
+  g_autoptr(GVariant) rval = ostree_async_progress_get_variant (self, key);
+  return (rval != NULL) ? g_variant_get_uint32 (rval) : 0;
+}
+
 guint64
 ostree_async_progress_get_uint64 (OstreeAsyncProgress       *self,
                                   const char                *key)
 {
-#if _OSTREE_HAVE_LP64
-  guint64 rval;
-  g_mutex_lock (&self->lock);
-  rval = (guint64) g_hash_table_lookup (self->uint64_values, GUINT_TO_POINTER (g_quark_from_string (key)));
-  g_mutex_unlock (&self->lock);
-  return rval;
-#else
-  guint64 *rval;
-  g_mutex_lock (&self->lock);
-  rval = g_hash_table_lookup (self->uint64_values, (gpointer)g_quark_from_string (key));
-  g_mutex_unlock (&self->lock);
-  if (rval)
-    return *rval;
-  return 0;
-#endif
+  g_autoptr(GVariant) rval = ostree_async_progress_get_variant (self, key);
+  return (rval != NULL) ? g_variant_get_uint64 (rval) : 0;
 }
 
 static gboolean
@@ -204,26 +205,45 @@ ostree_async_progress_get_status (OstreeAsyncProgress       *self)
   return ret;
 }
 
-static void
-update_key (OstreeAsyncProgress   *self,
-            GHashTable            *hash,
-            const char            *key,
-            gpointer               value)
+/**
+ * ostree_async_progress_set_variant:
+ * @self: an #OstreeAsyncProgress
+ * @key: a key to set
+ * @value: the value to assign to @key
+ *
+ * Assign a new @value to the given @key, replacing any existing value. The
+ * operation is thread-safe. @value may be a floating reference;
+ * g_variant_ref_sink() will be called on it.
+ *
+ * Any watchers of the #OstreeAsyncProgress will be notified of the change if
+ * @value differs from the existing value for @key.
+ *
+ * Since: 2017.6
+ */
+void
+ostree_async_progress_set_variant (OstreeAsyncProgress *self,
+                                   const char          *key,
+                                   GVariant            *value)
 {
-  gpointer orig_value;
+  GVariant *orig_value;
+  g_autoptr(GVariant) new_value = g_variant_ref_sink (value);
   gpointer qkey = GUINT_TO_POINTER (g_quark_from_string (key));
 
+  g_return_if_fail (OSTREE_IS_ASYNC_PROGRESS (self));
+  g_return_if_fail (key != NULL);
+  g_return_if_fail (value != NULL);
+
   g_mutex_lock (&self->lock);
 
   if (self->dead)
     goto out;
 
-  if (g_hash_table_lookup_extended (hash, qkey, NULL, &orig_value))
+  if (g_hash_table_lookup_extended (self->values, qkey, NULL, (gpointer *) &orig_value))
     {
-      if (orig_value == value)
+      if (g_variant_equal (orig_value, new_value))
         goto out;
     }
-  g_hash_table_replace (hash, qkey, value);
+  g_hash_table_replace (self->values, qkey, g_steal_pointer (&new_value));
   ensure_callback_locked (self);
 
  out:
@@ -235,7 +255,7 @@ ostree_async_progress_set_uint (OstreeAsyncProgress       *self,
                                 const char                *key,
                                 guint                      value)
 {
-  update_key (self, self->uint_values, key, GUINT_TO_POINTER (value));
+  ostree_async_progress_set_variant (self, key, g_variant_new_uint32 (value));
 }
 
 void
@@ -243,18 +263,7 @@ ostree_async_progress_set_uint64 (OstreeAsyncProgress       *self,
                                   const char                *key,
                                   guint64                    value)
 {
-  gpointer valuep;
-
-#if _OSTREE_HAVE_LP64
-  valuep = (gpointer)value;
-#else
-  {
-    guint64 *boxed = g_malloc (sizeof (guint64));
-    *boxed = value;
-    valuep = boxed;
-  }
-#endif
-  update_key (self, self->uint64_values, key, valuep);
+  ostree_async_progress_set_variant (self, key, g_variant_new_uint64 (value));
 }
 
 /**

diff --git a/src/libostree/ostree-async-progress.h b/src/libostree/ostree-async-progress.h
index ae0e5faab5fcf40229054298b566c6069788dc50..62bdcff87b30d43e28a3ea78b289ed512d24bea0 100644 (file)
--- a/src/libostree/ostree-async-progress.h
+++ b/src/libostree/ostree-async-progress.h
@@ -59,6 +59,9 @@ guint ostree_async_progress_get_uint (OstreeAsyncProgress       *self,
 _OSTREE_PUBLIC
 guint64 ostree_async_progress_get_uint64 (OstreeAsyncProgress       *self,
                                           const char                *key);
+_OSTREE_PUBLIC
+GVariant *ostree_async_progress_get_variant (OstreeAsyncProgress *self,
+                                             const char          *key);
 
 _OSTREE_PUBLIC
 void ostree_async_progress_set_status (OstreeAsyncProgress       *self,
@@ -72,6 +75,10 @@ _OSTREE_PUBLIC
 void ostree_async_progress_set_uint64 (OstreeAsyncProgress       *self,
                                        const char                *key,
                                        guint64                    value);
+_OSTREE_PUBLIC
+void ostree_async_progress_set_variant (OstreeAsyncProgress *self,
+                                        const char          *key,
+                                        GVariant            *value);
 
 _OSTREE_PUBLIC
 void ostree_async_progress_finish (OstreeAsyncProgress *self);